Skip to content

[SYSTEMDS-3899] Unary out-of-core operations#2298

Closed
j143 wants to merge 13 commits into
apache:mainfrom
j143:SYSTEMDS-3899-unary-out-of-core
Closed

[SYSTEMDS-3899] Unary out-of-core operations#2298
j143 wants to merge 13 commits into
apache:mainfrom
j143:SYSTEMDS-3899-unary-out-of-core

Conversation

@j143

@j143 j143 commented Jul 27, 2025

Copy link
Copy Markdown
Member

this one focuses on ceil(X) operation.

j143 added 4 commits July 27, 2025 12:37
Heavy hitter instructions:
 #  Instruction  Time(s)  Count
 1  ooc_rblk       0.055      1
 2  toString       0.049      2
 3  print          0.016      2
 4  createvar      0.013      3
 5  ooc_ceil       0.001      1
 6  rmvar          0.000      3

@mboehm7 mboehm7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @j143 - the code looks already pretty good, and here are a few pointers for improvements.

Comment thread src/main/java/org/apache/sysds/runtime/instructions/OOCInstructionParser.java Outdated
Comment thread src/main/java/org/apache/sysds/runtime/instructions/ooc/UnaryOOCInstruction.java Outdated
runTest(true, false, null, -1);

HashMap<MatrixValue.CellIndex, Double> dmlfile = readDMLMatrixFromOutputDir(OUTPUT_NAME);
// Double result = dmlfile.get(new MatrixValue.CellIndex(1, 1));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some check for result correctness would be good. You might use an aggregation and compare just the final scalar value.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I've done this.


# Read input matrix and operator from command line args
X = read($1);
print(toString(X))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

avoid because currently, we only support a single consumer on a streaming OOC input. The toString would collect the entire matrix.

@j143 j143 Jul 27, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noted. thanks

Y = ceil(X);
print(toString(Y))
# Write the final matrix result
write(Y, $2, format="binary");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the write is currently not yet supported so better use an aggregation and write the result as.matrix(agg)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was about to ask this! 😄

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. I've used sum aggregation and tested it against Java result.

@j143 j143 marked this pull request as ready for review July 27, 2025 09:47
@j143

j143 commented Jul 27, 2025

Copy link
Copy Markdown
Member Author

Hi @mboehm7 , thanks for the review. I've addressed the comments.

@mboehm7

mboehm7 commented Jul 27, 2025

Copy link
Copy Markdown
Contributor

LGTM - Thanks for the quick improvements @j143. During the merge I only slightly improved the test via (setExecMode for consistent handling).

@mboehm7 mboehm7 closed this in a5b298c Jul 27, 2025
@github-project-automation github-project-automation Bot moved this from In Progress to Done in SystemDS PR Queue Jul 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants